fix: address P1 security and performance findings#30
Merged
hybridindie merged 6 commits intomainfrom Mar 13, 2026
Merged
Conversation
- Check audit file path and parents for symlinks before writing - Create parent directories once at init instead of every write - Add async_log() method using asyncio.to_thread() for non-blocking writes - Sync log() method still available for non-async callers Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Reject empty/whitespace-only queries - Cap query length at 200 characters - Cap model_type length at 100 characters - Strip whitespace from query before forwarding to external APIs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Inject shared httpx.AsyncClient for external model searches instead of creating a new client per request (eliminates repeated TLS handshakes) - Use asyncio.gather() for concurrent HuggingFace detail fetches instead of sequential N+1 requests (reduces search latency from ~5.5s to ~1s) - Extract _fetch_hf_model_detail() helper for single-model detail fetch Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace synchronous audit.log() with await audit.async_log() across all 7 tool modules (56 call sites). This prevents blocking the async event loop on file I/O for every tool invocation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Collect unique folders needed across all model loader nodes - Fetch all folders concurrently with asyncio.gather() instead of sequential per-node HTTP calls - Deduplicate: two nodes loading from the same folder trigger only one API call (was O(N) per model loader, now O(unique folders)) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Addresses remaining P1 security/performance findings by hardening audit log path handling, improving model search/validation concurrency, and reducing external HTTP overhead via a shared httpx.AsyncClient.
Changes:
- Add input validation to
search_models, parallelize HuggingFace detail fetches, and inject a sharedhttpx.AsyncClientfor external model searches. - Harden
AuditLoggerwith symlink checks and introduce non-blockingasync_log(); migrate tool audit calls toawait audit.async_log(...). - Deduplicate + parallelize model folder checks in
validate_workflow, with corresponding test coverage.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/comfyui_mcp/audit.py |
Adds symlink protection and async audit logging via asyncio.to_thread(). |
src/comfyui_mcp/server.py |
Wires a shared external httpx.AsyncClient into model tools. |
src/comfyui_mcp/tools/models.py |
Validates search_models inputs, parallelizes HF details, uses injected HTTP client, migrates audit calls to async. |
src/comfyui_mcp/workflow/validation.py |
Batches model existence checks by folder and fetches folder listings in parallel. |
src/comfyui_mcp/tools/* (workflow.py, jobs.py, history.py, generation.py, files.py, discovery.py) |
Migrates tool audit calls from sync log() to async async_log(). |
tests/test_audit.py |
Adds tests for symlink protection and async logging concurrency. |
tests/test_tools_models.py |
Updates tool wiring for injected HTTP client; adds input validation + HF concurrency tests. |
tests/test_workflow_validation.py |
Adds coverage for parallel/deduped model folder checks in workflow validation. |
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
19
to
46
| @pytest.fixture | ||
| def components(tmp_path): | ||
| client = ComfyUIClient(base_url="http://test:8188") | ||
| audit = AuditLogger(audit_file=tmp_path / "audit.log") | ||
| read_limiter = RateLimiter(max_per_minute=60) | ||
| file_limiter = RateLimiter(max_per_minute=30) | ||
| sanitizer = PathSanitizer( | ||
| allowed_extensions=[".safetensors", ".ckpt", ".pt", ".pth", ".bin"], | ||
| max_size_mb=50, | ||
| ) | ||
| detector = ModelManagerDetector(client) | ||
| validator = DownloadValidator( | ||
| allowed_domains=["huggingface.co", "civitai.com"], | ||
| allowed_extensions=[".safetensors", ".ckpt", ".pt", ".pth", ".bin"], | ||
| ) | ||
| search_settings = ModelSearchSettings() | ||
| search_http = httpx.AsyncClient() | ||
| return { | ||
| "client": client, | ||
| "audit": audit, | ||
| "read_limiter": read_limiter, | ||
| "file_limiter": file_limiter, | ||
| "sanitizer": sanitizer, | ||
| "detector": detector, | ||
| "validator": validator, | ||
| "search_settings": search_settings, | ||
| "search_http": search_http, | ||
| } |
Comment on lines
+68
to
+79
| def _ensure_dir(self) -> bool: | ||
| """Create parent directories once. Returns False if path is unsafe.""" | ||
| if self._dir_created: | ||
| return True | ||
| if not self._is_path_safe(): | ||
| _logger.error("AUDIT LOG REFUSED: path contains symlink: %s", self._audit_file) | ||
| return False | ||
| try: | ||
| self._audit_file.parent.mkdir(parents=True, exist_ok=True) | ||
| self._dir_created = True | ||
| return True | ||
| except OSError as e: |
Comment on lines
+83
to
+106
| def _write_record(self, record: AuditRecord) -> None: | ||
| """Synchronous write of a single audit record.""" | ||
| if not self._ensure_dir(): | ||
| return | ||
| # Re-check symlink at write time (TOCTOU mitigation) | ||
| if self._audit_file.is_symlink(): | ||
| _logger.error("AUDIT LOG REFUSED: file is a symlink: %s", self._audit_file) | ||
| return | ||
| try: | ||
| with open(self._audit_file, "a") as f: | ||
| f.write(record.model_dump_json() + "\n") | ||
| except OSError as e: | ||
| import logging | ||
| _logger.error("AUDIT LOG FAILURE: %s", e) | ||
|
|
||
| logging.getLogger(__name__).error("AUDIT LOG FAILURE: %s", e) | ||
| def log(self, *, tool: str, action: str, **kwargs: Any) -> AuditRecord: | ||
| """Write an audit record as a JSON line (synchronous).""" | ||
| record = AuditRecord(tool=tool, action=action, **kwargs) | ||
| self._write_record(record) | ||
| return record | ||
|
|
||
| async def async_log(self, *, tool: str, action: str, **kwargs: Any) -> AuditRecord: | ||
| """Write an audit record without blocking the event loop.""" | ||
| record = AuditRecord(tool=tool, action=action, **kwargs) | ||
| await asyncio.to_thread(self._write_record, record) |
src/comfyui_mcp/server.py
Outdated
Comment on lines
144
to
193
| search_http=search_http, | ||
| ) | ||
|
|
||
| return server, settings |
- audit.py: re-check symlink safety on every write (not cached after first dir creation) to detect post-init symlink swaps - audit.py: add threading.Lock around _write_record() to prevent concurrent asyncio.to_thread() calls from interleaving writes - audit.py: is_symlink() catches dangling symlinks without exists() guard - server.py: register atexit handler to close client and search_http - server.py: return client/search_http from _build_server() for lifecycle - tests: add dangling symlink and post-write symlink swap test cases - tests: make components fixture async with search_http cleanup - tests: update all _build_server() unpackings for new return tuple Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the 6 remaining P1 (high priority) security and performance findings from the comprehensive code review:
search_modelsquery params (max length, empty check, whitespace strip)asyncio.gather()(reduces search latency from ~5.5s to ~1s)httpx.AsyncClientfor external model searches (eliminates per-call TLS handshakes)async_log()method usingasyncio.to_thread()and migrate all 56 tool audit call sitesvalidate_workflow(O(N) → O(unique folders))Test plan
uv run pytest -v— 371 tests pass, 92% coverage maintaineduv run pre-commit run --all-files— all hooks passsearch_models🤖 Generated with Claude Code